fix(backend): Synced ScheduledWorkflow CRs on apiserver startup #11469

hbelmiro · 2024-12-16T19:09:55Z

Resolves: #11296

This PR changes apiserver to patch existing ScheduledWorkflow CRs for each Job on startup to reflect the current KFP version deployed.

Testing

Create recurring runs
Make sure the recurring runs are running

Patch their swf CRs to force failures
3.1 Get the swf CRs

kubectl get swf
NAME                             AGE
heya8gqgg                        27m
howdyxt5fj                       26m
runofpipelinewithconditio5zwcj   60m

3.2 For each swf patch them with an invalid workflow spec to force failures. At this point, the recurring runs will start to fail due to the invalid spec.

kubectl patch scheduledworkflow heya8gqgg --type='merge' -p '{"spec":{"workflow":{"spec":{"dynamicKey1":"dynamicValue1","dynamicKey2":"dynamicValue2"}}}}'

Build a new apiserver image
Edit the ml-pipeline deployment to use the new apiserver image
The new apiserver pod will fix the swf CRs and the recurring runs will run successfully again

See a video demonstrating the test:

kfp-issue-11296.mp4

Checklist:

You have signed off your commits
The title for your pull request (PR) should follow our title convention. Learn more about the pull request title convention used in this repository.

google-oss-prow · 2024-12-16T19:09:59Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

hbelmiro · 2024-12-17T20:53:51Z

@HumairAK @chensun can you guys PTAL?

backend/src/apiserver/main.go

backend/src/apiserver/resource/resource_manager.go

mprahl · 2024-12-17T21:53:32Z

backend/src/apiserver/main.go

@@ -106,6 +106,13 @@ func main() {
 	}
 	log.SetLevel(level)

+	ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute)
+	defer cancel()
+	err = resourceManager.SyncSwfCrs(ctx)


The persistence agent seems to already have a reconcile loop for scheduled workflows. If I'm reading the code right, on start up, it'll reconcile everything and then handle creates, updates, and deletes.

Could the migration logic be added to the persistence agent instead?

It probably could. But that doesn't sound like a persistence agent's responsibility. It doesn't sound like API server's responsibility too, but I think it fits better there since we have a jobStore.

My original plan was to do it in https://github.com/kubeflow/pipelines/tree/master/backend/src/crd/controller/scheduledworkflow, but we would have to make HTTP calls to the API server. Then we decided to leave it in the API server.

It would be nice to hear others' opinions about that.

@hbelmiro I agree that persistence agent isn't the best fit. That controller you linked could be a good fit.

I think we should consider adding a KFP version column to the jobs table so that you can skip generating and updating scheduled workflows if the version matches.

If were to pursue using the scheduled workflow controller, here is an idea:

Have the scheduled workflow controller query the API server health endpoint at start up to get the tag_name value to see what version of KFP we are on. In the background, it could keep querying the API server to see if the version changed.

The scheduled workflow controller's reconcile loop checks an annotation of pipelines.kubeflow.org/version and if the value of that annotation doesn't match tag_name, then the workflow definition is updated and the annotation value is set to the current version.

When the API server creates a ScheduledWorkflow object, it sets the pipelines.kubeflow.org/version annotation.

I love this idea. My only concern is about making HTTP calls to the API server.
How about implementing it in a follow-up PR?

Sure, no problem! What do you think of the other comnment of adding a KFP version column to the jobs table so that you can skip generating and updating scheduled workflows if the version matches?

@mprahl where do you see it?

@mprahl @hbelmiro beware that, in the past, we had also problems with scheduled runs after upgrading RHOAI:

If I recall well, the problem in https://issues.redhat.com/browse/RHOAIENG-10790 was that the kfp-launcher pod needed an additional parameter to be set and the scheduled runs created before the upgrade didn't provide that parameter, so they failed to run. In this case it would have been helpful to rebuild all scheduled runs on kfp api server startup, regardless the kfp api server version.

What about providing an environment variable to control the behavior?
ML_PIPELINE_SCHEDULEDRUNS_REBUILDPOLICY=[disable,always,...]

Alternatively, we could set the API Server image name and tag from the Downward API. That way, downstream projects would also benefit even when the KFP version doesn't change.

@mprahl this discussion is still pending.

Since we're now checking if the current swf is deeply equal the new one, I think we could skip this verification.
We would still be fetching the current swf and generating the new one unnecessarily, but I think that's a fair price to avoid adding more complexity.

WDYT?

I'd like to have @jgarciao @HumairAK's opinion on this too.

(My opinion on this is not strong and I'm fine with implementing this if you guys think it's better)

My opinion is that the current code is good enough for now and the version metadata isn't needed at the moment. I think the extra version data would be nice but I'm thinking there isn't likely to be very many scheduled workflows, so the extra cost that happens asynchronously seems okay to me.

backend/src/apiserver/main.go

mprahl · 2024-12-18T15:25:59Z

backend/src/apiserver/main.go

+	defer cancel()
+	err := resourceManager.ReconcileSwfCrs(ctx)
+	if err != nil {
+		log.Errorf("Could not reconcile the ScheduledWorkflow Kubernetes resources: %v", err)


I agree with your approach of not exiting with an error code in this case, but I'm curious what others think. My concern is that it'd be hard to know that this failed as a KFP admin but it also doesn't seem warranted to keep the API server from running if this can't succeed.

If I was implementing this from scratch, I'd definitely exit with an error. But since the original behavior (the one that this PR aims to fix) is to run the API server with outdated swfs, existing deployments may start to fail after an upgrade even when they would work with outdated swfs.
At the same time, silently ignoring it and the outdated swfs "working" silently may be even more dangerous than just exiting with an error. (Now while I'm writing this, I'm tilting more to exit with an error)

Yeah, let's see what others think.

If the api server is still running users will be able to delete or clone the failing scheduled runs using the UI

That's a good point @jgarciao.

backend/src/apiserver/resource/resource_manager.go

hbelmiro · 2024-12-23T11:21:03Z

This PR depends on:

I included those commits here while the PRs are not merged.

cc @mprahl

backend/src/apiserver/resource/resource_manager.go

Signed-off-by: Helber Belmiro <[email protected]>

Co-authored-by: Matt Prahl <[email protected]> Signed-off-by: Helber Belmiro <[email protected]>

Signed-off-by: Helber Belmiro <[email protected]>

…the value is already correct? Signed-off-by: Helber Belmiro <[email protected]>

Signed-off-by: Helber Belmiro <[email protected]>

backend/src/apiserver/resource/resource_manager.go

mprahl · 2025-01-02T18:46:14Z

@hbelmiro is there any way to add test coverage for this?

Co-authored-by: Matt Prahl <[email protected]> Signed-off-by: Helber Belmiro <[email protected]>

hbelmiro · 2025-01-02T20:05:09Z

@hbelmiro is there any way to add test coverage for this?

@mprahl I'm not sure it's worth it. I'd have to replicate this reproducer, which means recreating the API Server pod. That would probably affect other tests.
Also, if we implement the version/image check, it will be even harder to test.

mprahl · 2025-01-02T20:44:34Z

@hbelmiro is there any way to add test coverage for this?

@mprahl I'm not sure it's worth it. I'd have to replicate this reproducer, which means recreating the API Server pod. That would probably affect other tests. Also, if we implement the version/image check, it will be even harder to test.

I was thinking you could have a test that calls ReconcileSwfCrs with the fake client manager. See backend/src/apiserver/resource/resource_manager_test.go for examples of where this is used. You should be able to create an invalid ScheduledWorkflow in the fake store (in memory SQLite database), call ReconcileSwfCrs, and verify it was updated.

Signed-off-by: Helber Belmiro <[email protected]>

hbelmiro · 2025-01-03T13:23:02Z

@hbelmiro is there any way to add test coverage for this?

@mprahl I'm not sure it's worth it. I'd have to replicate this reproducer, which means recreating the API Server pod. That would probably affect other tests. Also, if we implement the version/image check, it will be even harder to test.

I was thinking you could have a test that calls ReconcileSwfCrs with the fake client manager. See backend/src/apiserver/resource/resource_manager_test.go for examples of where this is used. You should be able to create an invalid ScheduledWorkflow in the fake store (in memory SQLite database), call ReconcileSwfCrs, and verify it was updated.

@mprahl actually what needs to be updated is the CR. But that's a good idea anyway. I've added the test.

mprahl

/lgtm

google-oss-prow · 2025-01-03T13:56:52Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mprahl
Once this PR has been reviewed and has the lgtm label, please assign chensun for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

backend/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

google-oss-prow bot added the do-not-merge/work-in-progress label Dec 16, 2024

google-oss-prow bot requested review from ouadakarim, rimolive and zijianjoy December 16, 2024 19:10

google-oss-prow bot added the size/XXL label Dec 16, 2024

hbelmiro force-pushed the issue-11296 branch 2 times, most recently from 8ed1796 to 332cc47 Compare December 16, 2024 19:27

google-oss-prow bot added size/L and removed size/XXL labels Dec 16, 2024

hbelmiro force-pushed the issue-11296 branch from 50713d3 to 953426d Compare December 17, 2024 20:17

hbelmiro changed the title ~~WIP - Update SWF CRs on apiserver startup~~ fix(backend): Synced ScheduledWorkflow CRs on apiserver startup Dec 17, 2024

hbelmiro marked this pull request as ready for review December 17, 2024 20:26

google-oss-prow bot removed the do-not-merge/work-in-progress label Dec 17, 2024

google-oss-prow bot requested a review from HumairAK December 17, 2024 20:26

mprahl reviewed Dec 17, 2024

View reviewed changes

backend/src/apiserver/main.go Outdated Show resolved Hide resolved

mprahl reviewed Dec 17, 2024

View reviewed changes

backend/src/apiserver/resource/resource_manager.go Outdated Show resolved Hide resolved

mprahl reviewed Dec 17, 2024

View reviewed changes